Backport tf multisensor fix #245
Conversation
|
I added this PR! Thank you for contributing the ROS 2 version of this, @nachovizzo . Hope I didn't work redundantly to anyone -- this addition was helpful for my work. I actually still have a question about the intended usage of the Even though the transformBroadcaster was sending a transform, that transform was neglected by Rviz, presumably because the sensor frame of my vehicle had already been given a parent by another odometry system. For me, Rviz could not visualize the transform from |
| roscpp | ||
| rosbag | ||
| std_msgs | ||
| tf2 | ||
| tf2_ros) | ||
| tf2_ros | ||
| Sophus) |
There was a problem hiding this comment.
From this one, I'm not quite sure what is the "best" practice... KISS-ICP has its own, isolated and self-contained build system... This means that the only way you couldn't "link" against on of its dependencies is when you copy-paste by hand some source files.
On the other hand, if we add sophus here, then, we should also add eigen, TBB, etc...
Do you have a better idea for this?
There was a problem hiding this comment.
Hmmm, that's a good question. I added the Sophus dependency because I got a linker error for Sophus when doing the following build process for ROS 1:
cd ~/catkin_ws/src
git clone https://github.com/PRBonn/kiss-icp
cd ~/catkin_ws
rosdep install --from-paths src --ignore-src -r -y
catkin build
# the compiler initially complained that Sophus could not be found
# but worked once I added the dependency to CMakeLists.txt and package.xml
I think that, because we explicitly include Sophus and Eigen headers in the OdometryServer code, we should have a dependency on these in package.xml and CMakeLists.txt. That way, if kiss-icp-cpp ever somehow removed its dependencies on these packages in the future (unlikely) then the ROS packages would still compile. It feels risky to include a dependency's dependency based on prior knowledge of the build process without explicitly adding it to the package.xml.
(I'm keen to know if there's a better way to do this too.)
There was a problem hiding this comment.
Linking error is unlikely since sophus is a header-only library. On the other hand, lets take for example tf2_ros, just because is the one above:
https://github.com/ros/geometry2/blob/309814fd7d55d874f9bac40b2cc8669a5b86f3c5/tf2_ros/CMakeLists.txt#L9
We don't need to include actionlib as our dependency. That's the whole magic of build systems.
On the other other hand, I also agree that just because we explicitly include the headers in the Odometryserver, that injects a hard dependency.... so it might be cleaner to explicitly put it there. My problem is with ROS-based build systems, that's if you put sophus on the package.xml and it's not in your system, then the build should fail. In contrast, KISS will fetch this at build time...
I'm puzzled about which is the best approach for this case. I will meditate and be back at this PR
|
@playertr would you mind updating this branch with the latest changes form the ros2 driver? Thanks! And sorry for making you work twice :( |
Sure, I'll rebase off of |
playertr
force-pushed
the
backport-tf-multisensor-fix
branch
from
42644ef
to
f1bf795
Compare
|
Merging this and finishing some bits at the big PR ;) Thanks a lot for the contribution! |
* Fix TF tree usage in ROS 2 wrapper Signed-off-by: Ignacio Vizzo <[email protected]> * Transform the pose instead of the pointcloud * remove include * Remove default base_frame parameter value This will make KISS-ICP work ego-centric as default * Remove unused variable * Do not reuse pose_msg.pose * I'm not ever sure why we did that in first instance * Be more explicit about the stamps and frame_id of the headers * Extract publishing mechanism to a new function (#246) * Convert class member function to more useful utility And fix a small bug, the order was of the transformation before was the opposite, and therefore we were obtaining base2cloud. Since we multiply by both sides we can't really see the difference, but it was conceptually wrong. * Bring back debugging clouds to ros driver * re-arange logic on when and when not to publish clouds * fix lofic * Update rviz2 * Loosing my mind already with this debugging info * Backport tf multisensor fix (#245) * Build system changes for tf fix * Modify params for tf fix * Add ROS 1 tf fixes similar to ROS 2 * Update rviz config * Remove unused debug publishers * Remove unnecessary smart pointers * Update ROS 1 to match ROS 2 changes * Fix style * Remove sophus from build system Fixing now the CI is a big pain * Remove unnecessary alias --------- Signed-off-by: Ignacio Vizzo <[email protected]> Co-authored-by: Tim Player <[email protected]> Co-authored-by: raw_t <[email protected]> Co-authored-by: tizianoGuadagnino <[email protected]>
Resolves #244 : backports the changes to the tf buffer from ROS 2 to ROS 1, with the eventual goal of supporting multi-sensor online odometry.
This PR also changes a few minor bugs in the ROS 2 implementation:
egocentric_estimationcomputationPlease check:
unique_ptrusage and move semantics are less idiomatic to ROS 1 than to ROS 2, but I think my implementation is legit/kissnamespace preface for our topics, but it might be more idiomatic to just put these nodes in a Namespace group in the launch file